Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature issue 376 #381

Closed
wants to merge 3 commits into from
Closed

Conversation

JingHuaMan
Copy link
Contributor

@JingHuaMan JingHuaMan commented Apr 18, 2021

#376

If the percentile is 0 or 1, it's unnecessary to sort all the elements in the container before getting the output by the index; instead, we can just scan all the items in one traversal and get the minimum or maximum with a time complexity of O(n).

All the tests passed after the modification. Besides, in the test testPercentileWithStringsAndFunction, since the function is String::length and each string in the stream is of length 1, the sorting result should be the same as the original order of the input elements.

I think the orginal test is not enough, so I add a new test for the function percentileBy.

Edit: add a second test.

@JingHuaMan JingHuaMan marked this pull request as draft April 23, 2021 13:57
@JingHuaMan JingHuaMan closed this Apr 23, 2021
@JingHuaMan JingHuaMan deleted the feature-issue-376 branch April 23, 2021 14:33
@lukaseder lukaseder added this to the Version 0.9.15 milestone May 21, 2021
@lukaseder
Copy link
Member

Thanks a lot for this suggestion. I agree with it. Will merge for inclusion in 0.9.15

@lukaseder
Copy link
Member

It seems I can't merge it anymore since you closed it. Is there any reason for your closing it? If I merge your PR, you'll get credit for it in the commit history. Otherwise, I'll go ahead and copy the contents...

@lukaseder
Copy link
Member

Nevermind. It seems you've submitted another PR later: #382

@lukaseder
Copy link
Member

Duplicate of #382

@lukaseder lukaseder marked this as a duplicate of #382 May 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants